Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update benchmarking + Support Android + add .swift-format + other refinements #227

Merged
merged 7 commits into from
Feb 14, 2025
Merged

Update benchmarking + Support Android + add .swift-format + other refinements #227

merged 7 commits into from
Feb 14, 2025

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Feb 11, 2025

No description provided.

@penny-for-vapor
Copy link

penny-for-vapor bot commented Feb 11, 2025 •
edited
Loading

Benchmark Report for ce64ff6

✅ Pull request has no significant performance differences ✅

Click to expand comparison result

Benchmark check running at 2025-02-14 16:36:46 UTC

The baseline 'Update benchmarking + Support Android + add .swift-format + other refinements' is EQUAL to the defined thresholds.

Click to expand benchmark result

Baseline 'Update benchmarking + Support Android + add .swift-format + other refinements'

Host '2162cd32897b' with 2 'aarch64' processors with 3 GB memory, running:
#23-Ubuntu SMP Mon Dec  9 23:51:16 UTC 2024

Signing

ES256

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 169 169 203 203 271 373 475 2694
Memory (resident peak) (M) 26 27 27 27 27 27 27 2694

EdDSA

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 117 117 117 117 117 117 117 4515
Memory (resident peak) (M) 26 27 27 27 27 27 27 4515

RSA

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 239 239 239 248 248 248 248 732
Memory (resident peak) (M) 26 27 27 27 27 27 27 732

TokenLifecycle

ES256 Generated

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 355 355 355 389 457 559 695 2202
Memory (resident peak) (M) 27 27 27 27 27 27 27 2202

ES256 PEM

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 373 373 373 373 373 373 373 3403
Memory (resident peak) (M) 27 27 27 27 27 27 27 3403

EdDSA Coordinates

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 323 323 323 323 323 323 323 2887
Memory (resident peak) (M) 27 28 28 28 28 28 28 2887

EdDSA Generated

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 291 291 291 291 291 291 291 3045
Memory (resident peak) (M) 26 27 27 27 27 27 27 3045

RSA PEM

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 436 436 436 445 445 445 445 693
Memory (resident peak) (M) 27 27 27 28 28 28 28 693

Verifying

ES256

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 263 263 263 263 263 263 263 4187
Memory (resident peak) (M) 26 27 27 27 27 27 27 4187

EdDSA

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 251 251 251 251 251 251 251 3550
Memory (resident peak) (M) 27 27 27 27 27 27 27 3550

RS256

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 257 257 257 257 257 257 257 4194
Memory (resident peak) (M) 27 28 28 28 28 28 28 4194

MahdiBM
MahdiBM commented Feb 11, 2025
View reviewed changes
Benchmarks/Package.swift
@@ -18,7 +18,7 @@ let package = Package(
.product(name: "Benchmark", package: "package-benchmark"),
.product(name: "JWTKit", package: "jwt-kit"),
],
path: "Benchmarks/Signing",
path: "Signing",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made these the same as multipart-kit

MahdiBM
MahdiBM commented Feb 11, 2025
View reviewed changes
Benchmarks/.gitignore
@@ -0,0 +1 @@
../.gitignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were some .index-build stuff trying to slip in

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Feb 11, 2025

@marcprux trying to enable the android unit-tests, we're getting:

error: no such module 'Testing'

WDYT? Is this normal? Should we have android unit tests disabled for now?

@marcprux
Copy link

Should we have android unit tests disabled for now?

The Swift Testing framework is WIP right now. What I've been doing for other PRs is to guard the test cases in #if canImport(Testing), so once we release the next version of the Android SDK with Testing available, they will automatically start working.

Or your could back-port all your Testing tests to XCTest (which does work fine), but I wouldn't recommend 🫠

@codecov Codecov
Copy link

codecov bot commented Feb 11, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.79%. Comparing base (7bd6ecb) to head (ce64ff6).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #227   +/-   ##
=======================================
  Coverage   83.79%   83.79%           
=======================================
  Files          56       56           
  Lines        1493     1493           
=======================================
  Hits         1251     1251           
  Misses        242      242
Files with missing lines Coverage Δ
Sources/JWTKit/Claims/ExpirationClaim.swift 100.00% <ø> (ø)
Sources/JWTKit/Claims/IssuedAtClaim.swift 100.00% <ø> (ø)
Sources/JWTKit/Claims/JWTMultiValueClaim.swift 68.51% <ø> (ø)
Sources/JWTKit/Claims/JWTUnixEpochClaim.swift 100.00% <ø> (ø)
Sources/JWTKit/Claims/LocaleClaim.swift 100.00% <ø> (ø)
Sources/JWTKit/Claims/NotBeforeClaim.swift 25.00% <ø> (ø)
Sources/JWTKit/ECDSA/ECDSA.swift 87.03% <ø> (ø)
Sources/JWTKit/ECDSA/ECDSAKeyTypes.swift 100.00% <ø> (ø)
Sources/JWTKit/ECDSA/ECDSASigner.swift 90.90% <ø> (ø)
Sources/JWTKit/ECDSA/P256+CurveType.swift 100.00% <ø> (ø)
... and 24 more

@MahdiBM MahdiBM force-pushed the mmbm-benchmark-and-refinements branch from 884eaef to 39319c6 Compare February 11, 2025 17:01
MahdiBM
MahdiBM commented Feb 11, 2025
View reviewed changes
.swift-format
@@ -0,0 +1,70 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For vscode users

@MahdiBM MahdiBM changed the title Update benchmarking + other refinements Update benchmarking + Support Android + add .swift-format + other refinements Feb 11, 2025
@MahdiBM MahdiBM requested a review from ptoffy February 11, 2025 17:10
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Feb 11, 2025

@gwynne can we have android and benchmarks CI marked as required?

@gwynne
Copy link
Member

gwynne commented Feb 11, 2025

@gwynne can we have android and benchmarks CI marked as required?

Done

ptoffy
ptoffy approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@ptoffy ptoffy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry thought I had approved this already. Looks good but I'd honestly remove indentation in the tests here too

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Feb 14, 2025

@ptoffy you had but I re-requested a review since there were a lot of new changes.

@MahdiBM MahdiBM merged commit d5c5e02 into main Feb 14, 2025
15 checks passed
@MahdiBM MahdiBM deleted the mmbm-benchmark-and-refinements branch February 14, 2025 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptoffy ptoffy ptoffy approved these changes

@0xTim 0xTim Awaiting requested review from 0xTim 0xTim is a code owner

@gwynne gwynne Awaiting requested review from gwynne gwynne is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MahdiBM @marcprux @gwynne @ptoffy